-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Security Solution] Migrates siem-detection-engine-rule-status alertId to saved object references array #114585
[Security Solution] Migrates siem-detection-engine-rule-status alertId to saved object references array #114585
Conversation
…detection-engine-rule-status-references
…to references for create API
…detection-engine-rule-status-references
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
…detection-engine-rule-status-references
…detection-engine-rule-status-references
}, | ||
aggs: { | ||
rule_status: { | ||
reverse_nested: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ended up running into issues going the two query route as well too as it wasn't possible to do a nested
top_hits
agg and sort on a parent field without specifying reverse_nested
.... 😅
Touched base w/ @XavierM and just ended up adding support for reverse_nested
here. So now we're still able to complete this operation in a single query, and can keep the same extractions logic below as before. 🙂
create: ( | ||
attributes: IRuleStatusSOAttributes, | ||
options?: SavedObjectsCreateOptions | ||
) => Promise<SavedObject<IRuleStatusSOAttributes>>; | ||
update: ( | ||
id: string, | ||
attributes: Partial<IRuleStatusSOAttributes> | ||
attributes: Partial<IRuleStatusSOAttributes>, | ||
options?: SavedObjectsCreateOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create
/Update
interfaces updated to include SavedObjectsCreateOptions
so the SO references[]
can be supplied and the correct document is updated (otherwise we'll end up with new docs w/o references[]
).
…nt is always updated/created
export const truncateMessageFields: SavedObjectMigrationFn<Record<string, unknown>> = (doc) => { | ||
const { lastFailureMessage, lastSuccessMessage, ...restAttributes } = doc.attributes; | ||
|
||
return { | ||
...doc, | ||
attributes: { | ||
lastFailureMessage: truncateMessage(lastFailureMessage), | ||
lastSuccessMessage: truncateMessage(lastSuccessMessage), | ||
...restAttributes, | ||
}, | ||
references: doc.references ?? [], | ||
}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From recently merged #112257 -- copied over as part of breaking out this and the security-rule
SO mappings .
export const ruleStatusSavedObjectMappings: SavedObjectsType['mappings'] = { | ||
properties: { | ||
status: { | ||
type: 'keyword', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up removing the alertId
mapping. Spoke with @FrankHassanabad about this and he ended up leaving the mapping in some instances and just removed the field from the usable types. We should be fine with it removed here (and the types), but something to note when testing. Also means you can't use the painless
script to downgrade and test as there will be a mapping mis-match.
@@ -111,7 +111,6 @@ export type RuleAlertType = SanitizedAlert<RuleParams>; | |||
|
|||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
export interface IRuleStatusSOAttributes extends Record<string, any> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I attempted to remove extends Record<string, any>
as this is polluting this interface downstream and preventing type errors from invalid fields (i.e. alertId
), but ran into issues trying to resolve the types within mergeStatuses
and other utilities. Would be happy to pair and try to clean this up, but we're close to removing this SO and related utilities, so not a must unless time permits.
kibana/x-pack/plugins/security_solution/server/lib/detection_engine/routes/utils.ts
Line 208 in de43a3b
export const mergeStatuses = ( |
...etection_engine/rule_execution_log/saved_objects_adapter/rule_status_saved_objects_client.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for core changes
...etection_engine/rule_execution_log/saved_objects_adapter/rule_status_saved_objects_client.ts
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/routes/utils.test.ts
Show resolved
Hide resolved
@elasticmachine merge upstream |
Resolved a merge conflict ⬆️ in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks really good to me, thank you for figuring out the problem with nested/reverse_nested aggregations and for making the code cleaner 👍
I noticed what I believe is a bug, I'll try to quickly push a fix in this branch if I can.
return { | ||
...doc, | ||
attributes: { | ||
...attributesWithoutAlertId.attributes, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I think it should be attributes: attributesWithoutAlertId
. attributesWithoutAlertId.attributes
will be undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch -- thanks @banderror! Looks like this was a ©️🍝 from Frank's implementation. Here's the follow-up PR they did which includes the fix/test: https://github.com/elastic/kibana/pull/115098/files#diff-9729eb10e272059f7d9571cb208769b3e93fb73a5ac8483128d16dfc577e7057L248.
// early return if alertId is not a string as expected | ||
return { ...doc, references: existingReferences }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: return doc;
? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still want this otherwise references
won't be an empty array since it's absent in the original doc, no?
I'm not sure if the platform treats empty array references
and undefined the same, so would test before making the change.
@@ -86,6 +86,33 @@ export default ({ getService }: FtrProviderContext): void => { | |||
'7d' | |||
); | |||
}); | |||
|
|||
it('migrates legacy siem-detection-engine-rule-status to use saved object references', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another test checking that all the other attributes are migrated successfully would help to catch the bug in the migration function.
There's a similar one for siem-detection-engine-rule-actions
:
it('migrates legacy siem-detection-engine-rule-actions and retains "ruleThrottle" and "alertThrottle" as the same attributes as before', async () => {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there's an upcoming Jenkins upgrade in less than 2 hours:
The update process does cause downtime, stopping new builds from running and waiting as necessary for running jobs to complete before updating.
I'd like to merge this PR as soon as CI is 🟢 -- really need these changes in master for my own branch.
I'll submit the fix and a test for it in a separate PR.
Thanks again 🙏
💚 Build SucceededMetrics [docs]Saved Objects .kibana field count
History
To update your PR or re-run it, just comment with: cc @spong |
Auto-backport didn't work. I guess it's because I was not the author of the PR and merged it? 🤔 |
Follow-up fix here: #115355 |
…d to saved object references array (elastic#114585) ## Summary Resolves (a portion of) elastic#107068 for the `siem-detection-engine-rule-status` type by migrating the `alertId` to be within the `SO references[]`. Based on: elastic#113577 * Migrates the legacy `siem-detection-engine-rule-status` `alertId` to saved object references array * Adds an e2e test for `siem-detection-engine-rule-status` * Breaks out `siem-detection-engine-rule-status` & `security-rule` SO's to their own dedicated files/directories, and cleaned up typings/imports Before migration you can observe the existing data structure of `siem-detection-engine-rule-status` via Dev tools as follows: ``` GET .kibana/_search { "size": 10000, "query": { "term": { "type": { "value": "siem-detection-engine-rule-status" } } } } ``` ``` JSON { "_index" : ".kibana-spong_8.0.0_001", "_id" : "siem-detection-engine-rule-status:d580f1a0-2afe-11ec-8621-8d6bfcdfd75e", "_score" : 2.150102, "_source" : { "siem-detection-engine-rule-status" : { "alertId" : "d62d2980-27c4-11ec-92b0-f7b47106bb35", <-- alertId which we want in the references array and removed "statusDate" : "2021-10-12T01:50:52.898Z", "status" : "failed", "lastFailureAt" : "2021-10-12T01:50:52.898Z", "lastSuccessAt" : "2021-10-12T01:18:29.195Z", "lastFailureMessage" : "6 minutes (385585ms) were not queried between this rule execution and the last execution, so signals may have been missed. Consider increasing your look behind time or adding more Kibana instances. name: \"I am the Host who Names!\" id: \"d62d2980-27c4-11ec-92b0-f7b47106bb35\" rule id: \"214ccef6-e98e-493a-98c5-5bcc2d497b79\" signals index: \".siem-signals-spong-default\"", "lastSuccessMessage" : "succeeded", "gap" : "6 minutes", "lastLookBackDate" : "2021-10-07T23:43:27.961Z" }, "type" : "siem-detection-engine-rule-status", "references" : [ ], "coreMigrationVersion" : "7.14.0", "updated_at" : "2021-10-12T01:50:53.404Z" } } ``` Post migration the data structure should be updated as follows: ``` JSON { "_index": ".kibana-spong_8.0.0_001", "_id": "siem-detection-engine-rule-status:d580f1a0-2afe-11ec-8621-8d6bfcdfd75e", "_score": 2.1865466, "_source": { "siem-detection-engine-rule-status": { "statusDate": "2021-10-12T01:50:52.898Z", <-- alertId is no more! "status": "failed", "lastFailureAt": "2021-10-12T01:50:52.898Z", "lastSuccessAt": "2021-10-12T01:18:29.195Z", "lastFailureMessage": "6 minutes (385585ms) were not queried between this rule execution and the last execution, so signals may have been missed. Consider increasing your look behind time or adding more Kibana instances. name: \"I am the Host who Names!\" id: \"d62d2980-27c4-11ec-92b0-f7b47106bb35\" rule id: \"214ccef6-e98e-493a-98c5-5bcc2d497b79\" signals index: \".siem-signals-spong-default\"", "lastSuccessMessage": "succeeded", "gap": "6 minutes", "lastLookBackDate": "2021-10-07T23:43:27.961Z" }, "type": "siem-detection-engine-rule-status", "references": [ { "id": "d62d2980-27c4-11ec-92b0-f7b47106bb35", <-- previous alertId has been converted to references[] "type": "alert", "name": "alert_0" } ], "migrationVersion": { "siem-detection-engine-rule-status": "7.16.0" }, "coreMigrationVersion": "8.0.0", "updated_at": "2021-10-12T01:50:53.406Z" } }, ``` #### Manual testing --- There are e2e tests but for any manual testing or verification you can do the following: ##### Manual upgrade test If you have a 7.15.0 system and can migrate it forward that is the most straight forward way to ensure this does migrate correctly. You should see that the `Rule Monitoring` table and Rule Details `Failure History` table continue to function without error. ##### Downgrade via script and test migration on kibana reboot If you have a migrated `Rule Status SO` and want to test the migration, you can run the below script to downgrade the status SO then restart Kibana and observe the migration on startup. Note: Since this PR removes the mapping, you would need to [update the SO mapping](https://github.com/elastic/kibana/pull/114585/files#r729386126) to include `alertId` again else you will receive a strict/dynamic mapping error. ```json # Replace id w/ correct Rule Status SO id of existing migrated object POST .kibana/_update/siem-detection-engine-rule-status:d580ca91-2afe-11ec-8621-8d6bfcdfd75e { "script" : { "source": """ ctx._source.migrationVersion['siem-detection-engine-rule-status'] = "7.15.0"; ctx._source['siem-detection-engine-rule-status'].alertId = ctx._source.references[0].id; ctx._source.references.remove(0); """, "lang": "painless" } } ``` Restart Kibana and now it should be migrated correctly and you shouldn't see any errors in your console. You should also see that the `Rule Monitoring` table and Rule Details `Failure History` table continue to function without error. ### Checklist Delete any items that are not applicable to this PR. - [ ] ~[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials~ - [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…d to saved object references array (#114585) (#115359) ## Summary Resolves (a portion of) #107068 for the `siem-detection-engine-rule-status` type by migrating the `alertId` to be within the `SO references[]`. Based on: #113577 * Migrates the legacy `siem-detection-engine-rule-status` `alertId` to saved object references array * Adds an e2e test for `siem-detection-engine-rule-status` * Breaks out `siem-detection-engine-rule-status` & `security-rule` SO's to their own dedicated files/directories, and cleaned up typings/imports Before migration you can observe the existing data structure of `siem-detection-engine-rule-status` via Dev tools as follows: ``` GET .kibana/_search { "size": 10000, "query": { "term": { "type": { "value": "siem-detection-engine-rule-status" } } } } ``` ``` JSON { "_index" : ".kibana-spong_8.0.0_001", "_id" : "siem-detection-engine-rule-status:d580f1a0-2afe-11ec-8621-8d6bfcdfd75e", "_score" : 2.150102, "_source" : { "siem-detection-engine-rule-status" : { "alertId" : "d62d2980-27c4-11ec-92b0-f7b47106bb35", <-- alertId which we want in the references array and removed "statusDate" : "2021-10-12T01:50:52.898Z", "status" : "failed", "lastFailureAt" : "2021-10-12T01:50:52.898Z", "lastSuccessAt" : "2021-10-12T01:18:29.195Z", "lastFailureMessage" : "6 minutes (385585ms) were not queried between this rule execution and the last execution, so signals may have been missed. Consider increasing your look behind time or adding more Kibana instances. name: \"I am the Host who Names!\" id: \"d62d2980-27c4-11ec-92b0-f7b47106bb35\" rule id: \"214ccef6-e98e-493a-98c5-5bcc2d497b79\" signals index: \".siem-signals-spong-default\"", "lastSuccessMessage" : "succeeded", "gap" : "6 minutes", "lastLookBackDate" : "2021-10-07T23:43:27.961Z" }, "type" : "siem-detection-engine-rule-status", "references" : [ ], "coreMigrationVersion" : "7.14.0", "updated_at" : "2021-10-12T01:50:53.404Z" } } ``` Post migration the data structure should be updated as follows: ``` JSON { "_index": ".kibana-spong_8.0.0_001", "_id": "siem-detection-engine-rule-status:d580f1a0-2afe-11ec-8621-8d6bfcdfd75e", "_score": 2.1865466, "_source": { "siem-detection-engine-rule-status": { "statusDate": "2021-10-12T01:50:52.898Z", <-- alertId is no more! "status": "failed", "lastFailureAt": "2021-10-12T01:50:52.898Z", "lastSuccessAt": "2021-10-12T01:18:29.195Z", "lastFailureMessage": "6 minutes (385585ms) were not queried between this rule execution and the last execution, so signals may have been missed. Consider increasing your look behind time or adding more Kibana instances. name: \"I am the Host who Names!\" id: \"d62d2980-27c4-11ec-92b0-f7b47106bb35\" rule id: \"214ccef6-e98e-493a-98c5-5bcc2d497b79\" signals index: \".siem-signals-spong-default\"", "lastSuccessMessage": "succeeded", "gap": "6 minutes", "lastLookBackDate": "2021-10-07T23:43:27.961Z" }, "type": "siem-detection-engine-rule-status", "references": [ { "id": "d62d2980-27c4-11ec-92b0-f7b47106bb35", <-- previous alertId has been converted to references[] "type": "alert", "name": "alert_0" } ], "migrationVersion": { "siem-detection-engine-rule-status": "7.16.0" }, "coreMigrationVersion": "8.0.0", "updated_at": "2021-10-12T01:50:53.406Z" } }, ``` #### Manual testing --- There are e2e tests but for any manual testing or verification you can do the following: ##### Manual upgrade test If you have a 7.15.0 system and can migrate it forward that is the most straight forward way to ensure this does migrate correctly. You should see that the `Rule Monitoring` table and Rule Details `Failure History` table continue to function without error. ##### Downgrade via script and test migration on kibana reboot If you have a migrated `Rule Status SO` and want to test the migration, you can run the below script to downgrade the status SO then restart Kibana and observe the migration on startup. Note: Since this PR removes the mapping, you would need to [update the SO mapping](https://github.com/elastic/kibana/pull/114585/files#r729386126) to include `alertId` again else you will receive a strict/dynamic mapping error. ```json # Replace id w/ correct Rule Status SO id of existing migrated object POST .kibana/_update/siem-detection-engine-rule-status:d580ca91-2afe-11ec-8621-8d6bfcdfd75e { "script" : { "source": """ ctx._source.migrationVersion['siem-detection-engine-rule-status'] = "7.15.0"; ctx._source['siem-detection-engine-rule-status'].alertId = ctx._source.references[0].id; ctx._source.references.remove(0); """, "lang": "painless" } } ``` Restart Kibana and now it should be migrated correctly and you shouldn't see any errors in your console. You should also see that the `Rule Monitoring` table and Rule Details `Failure History` table continue to function without error. ### Checklist Delete any items that are not applicable to this PR. - [ ] ~[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials~ - [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [x] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Garrett Spong <spong@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…le-status Saved Object migration to SO references (#115355) **Ticket:** #107068 **Follow-up after:** #114585 ## Summary The existing migration function `legacyMigrateRuleAlertIdSOReferences` that migrates `alertId` fields to SO references array did not include all the other attributes of a `siem-detection-engine-rule-status` doc being migrated to the resulting doc. This PR includes a fix and an integration test for that. ## Run the test To run the test, in one terminal execute: ``` cd ${KIBANA_HOME} && node scripts/functional_tests_server --config x-pack/test/detection_engine_api_integration/security_and_spaces/config.ts ``` In another terminal execute: ``` cd ${KIBANA_HOME} && node scripts/functional_test_runner --config x-pack/test/detection_engine_api_integration/security_and_spaces/config.ts --include=x-pack/test/detection_engine_api_integration/security_and_spaces/tests/migrations.ts ``` ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
…le-status Saved Object migration to SO references (elastic#115355) **Ticket:** elastic#107068 **Follow-up after:** elastic#114585 ## Summary The existing migration function `legacyMigrateRuleAlertIdSOReferences` that migrates `alertId` fields to SO references array did not include all the other attributes of a `siem-detection-engine-rule-status` doc being migrated to the resulting doc. This PR includes a fix and an integration test for that. ## Run the test To run the test, in one terminal execute: ``` cd ${KIBANA_HOME} && node scripts/functional_tests_server --config x-pack/test/detection_engine_api_integration/security_and_spaces/config.ts ``` In another terminal execute: ``` cd ${KIBANA_HOME} && node scripts/functional_test_runner --config x-pack/test/detection_engine_api_integration/security_and_spaces/config.ts --include=x-pack/test/detection_engine_api_integration/security_and_spaces/tests/migrations.ts ``` ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
…le-status Saved Object migration to SO references (#115355) (#115486) **Ticket:** #107068 **Follow-up after:** #114585 ## Summary The existing migration function `legacyMigrateRuleAlertIdSOReferences` that migrates `alertId` fields to SO references array did not include all the other attributes of a `siem-detection-engine-rule-status` doc being migrated to the resulting doc. This PR includes a fix and an integration test for that. ## Run the test To run the test, in one terminal execute: ``` cd ${KIBANA_HOME} && node scripts/functional_tests_server --config x-pack/test/detection_engine_api_integration/security_and_spaces/config.ts ``` In another terminal execute: ``` cd ${KIBANA_HOME} && node scripts/functional_test_runner --config x-pack/test/detection_engine_api_integration/security_and_spaces/config.ts --include=x-pack/test/detection_engine_api_integration/security_and_spaces/tests/migrations.ts ``` ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co>
Summary
Resolves (a portion of) #107068 for the
siem-detection-engine-rule-status
type by migrating thealertId
to be within theSO references[]
. Based on: #113577siem-detection-engine-rule-status
alertId
to saved object references arraysiem-detection-engine-rule-status
siem-detection-engine-rule-status
&security-rule
SO's to their own dedicated files/directories, and cleaned up typings/importsBefore migration you can observe the existing data structure of
siem-detection-engine-rule-status
via Dev tools as follows:Post migration the data structure should be updated as follows:
Manual testing
There are e2e tests but for any manual testing or verification you can do the following:
Manual upgrade test
If you have a 7.15.0 system and can migrate it forward that is the most straight forward way to ensure this does migrate correctly. You should see that the
Rule Monitoring
table and Rule DetailsFailure History
table continue to function without error.Downgrade via script and test migration on kibana reboot
If you have a migrated
Rule Status SO
and want to test the migration, you can run the below script to downgrade the status SO then restart Kibana and observe the migration on startup.Note: Since this PR removes the mapping, you would need to update the SO mapping to include
alertId
again else you will receive a strict/dynamic mapping error.Restart Kibana and now it should be migrated correctly and you shouldn't see any errors in your console. You should also see that the
Rule Monitoring
table and Rule DetailsFailure History
table continue to function without error.Checklist
Delete any items that are not applicable to this PR.
Documentation was added for features that require explanation or tutorialsFor maintainers